Simplify php_openssl_load_all_certs_from_file()#21430
Simplify php_openssl_load_all_certs_from_file()#21430botovq wants to merge 1 commit intophp:masterfrom
Conversation
Correctly free both stacks, sk and stack, in the exit path and transfer ownership of stack to ret on success. Don't free the members of sk in the loop, only steal the certs after successful sk_X509_push(), which is error checked as it always should be. Switch from sk_new_reserve() back to the much saner sk_X509_new_null(). This makes ownership in this code easier to reason about and more robust to future modifications.
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
Looks good except that sk_X509_new_null
| } | ||
|
|
||
| if(!(stack = sk_X509_new_reserve(NULL, sk_X509_INFO_num(sk)))) { | ||
| if(!(stack = sk_X509_new_null())) { |
There was a problem hiding this comment.
Why is this changed? sk_X509_new_reserve pre-allocates space for the certs which should be better so don't exactly get why it was replaced...
There was a problem hiding this comment.
To be honest, I took a look at this because LibreSSL doesn't have sk_reserve and really doesn't want to add it if at all possible. There are currently no unconditional uses of this API and it would be nice if it stayed this way. It is a pretty bad interface that tends to attracts bugs (like in the case at hand) because whenever you think you need it, you almost certainly have ownership handling that could be improved instead.
So I thought I'd simply switch it back to what it was before 4b9e80e because I assumed the use of sk_reserve() was precisely to avoid having to error check sk_push().
| while (sk_X509_INFO_num(sk)) { | ||
| xi=sk_X509_INFO_shift(sk); | ||
| for (i = 0; i < sk_X509_INFO_num(sk); i++) { | ||
| xi=sk_X509_INFO_value(sk,i); |
There was a problem hiding this comment.
| xi=sk_X509_INFO_value(sk,i); | |
| xi = sk_X509_INFO_value(sk,i); |
when you are in it...
3 participants
Correctly free both stacks, sk and stack, in the exit path and transfer ownership of stack to ret on success. Don't free the members of sk in the loop, only steal the certs after successful sk_X509_push(), which is error checked as it always should be. Switch from sk_new_reserve() back to the much saner sk_X509_new_null(). This makes ownership in this code easier to reason about and more robust to future modifications.
Alternative to #21418